Explicit parameter binding - #1018
Conversation
| { | ||
| public class BoundValue | ||
| { | ||
| // ?? Why have an internal constructor on a public readonly class? |
There was a problem hiding this comment.
We don't want people to create instances of this.
There was a problem hiding this comment.
It does not appear that BoundValue escape the context of System.CommandLine. I think we should deliberately consider the needs of any application models that need something other than to connect a ValueSource to a Option/Argument ahead of any evaluation. IOW, we may need this public in the future, but it does not appear that we need it public now.
This is probably part of a broader discussion of checking our public API surface.
| { | ||
| public static class InvocationExtensions | ||
| { | ||
| public static void BindParameter(this ICommandHandler handler, ParameterInfo param, Option option) |
There was a problem hiding this comment.
These shouldn't be necessary, correct?
There was a problem hiding this comment.
I think it is needed, although it might not be in the right place. That extension method drives the following syntax:
handler.BindParameter(parameter, argument);
command.Handler = handler;
The handler is managed as an ICommandHandler, so putting the method on the interface isn't sensible due to the ParameterInfo/PropertyInfo. Thus, an extension method seemed the right approach.
There was a problem hiding this comment.
Or were you just expecting to always do it on the argument and this is on the option?
There was a problem hiding this comment.
I think making ModelBindingCommandHandler public makes more sense than extending the interface but throwing if it's the wrong implementation.
|
Closing in favor of non-draft one. |
2 participants
Significant updates to binding so method binding and model binding use the same logic and same code paths.
Draft due to rebase issues.